Expose generate_velocity_model_parameters so that you do not have to write to realisation#81
Expose generate_velocity_model_parameters so that you do not have to write to realisation#81lispandfound wants to merge 2 commits intopegasusfrom
generate_velocity_model_parameters so that you do not have to write to realisation#81Conversation
…ty_model_parameters` so that you do not have to write to realisation
Summary of ChangesHello @lispandfound, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the process of generating velocity model parameters by decoupling the core computation logic from file input/output operations. The primary function responsible for this generation now directly accepts necessary configuration objects and returns the computed domain parameters, promoting a more modular and testable design. A new command-line entrypoint has been added to maintain the existing file-based workflow, which now orchestrates the data loading, calls the refactored core function, and handles the result persistence. This enhancement streamlines the parameter generation process, making it more flexible for integration into various workflows. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors generate_velocity_model_parameters.py to separate the core logic from file I/O. The generate_velocity_model_parameters function is now a pure function, taking configuration objects as arguments and returning DomainParameters. This is a great improvement for testability and reusability. A new main function handles the command-line interface aspects, including reading from and writing to the realisation file. The changes are well-structured and clear. I've identified a couple of areas for improvement: an inefficiency in how the realisation file is read multiple times, and a minor inaccuracy in a docstring. Addressing these would make the implementation even more robust.
There was a problem hiding this comment.
Pull request overview
This PR refactors the generate_velocity_model_parameters module to separate the core domain generation logic from file I/O operations. The refactoring enables programmatic use of domain generation with in-memory configuration objects, removing the requirement to serialize realisation data to JSON files before invoking domain computation.
Key Changes:
- Extracted core domain computation logic into a library function
generate_velocity_model_parametersthat accepts configuration objects as parameters and returnsDomainParameters - Created a new
mainfunction that preserves the original CLI behavior by reading from realisation files, calling the refactored function, and writing results back - Removed duplicate code that was redundantly reading rakes configuration from the realisation file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
lispandfound
requested review from
AndrewRidden-Harper,
claudio525,
joelridden and
sungeunbae
|
|
||
| @cli.from_docstring(app) | ||
| @log_utils.log_call() | ||
| def generate_velocity_model_parameters( |
There was a problem hiding this comment.
I think it would be a good idea to put this into a module instead of leaving it as part of the script?
|
Closed in favour of #83 which also includes the rrup simplification. |
Simple change. Currently the only way to generate a domain is first write a realisation file and then execute the generate velocity model parameters script. This small refactor changes this so that domain generation can be invoked with simple realisation objects and without JSON serialisation.